Added a progress bar directive, with tests and docs #1389
Conversation
|
Thanks for the PR, but I can't review it as you've committed the build files, please remove them and squash your commits into a single one with a proper message format (check the guidelines). |
add a directive for a progress bar, with tests and docs resolve mgcrea#379
|
@mgcrea Done, hadn't seen the contributing guidelines sorry |
|
Does this allows using progress bar along with buttons? ex: kind of https://github.com/remotty/angular-ladda progress. Not exactly like this module, but will this allows adding progress bar to any other directive too? Thanks. |
|
@manikantag The progress bar directive compiles to a div, so it should work within a button yes |
|
oh, good then. |
|
@mgcrea This looks good, should it be merged? |
|
@vmlf01, I'd like to merge this, but it feels like it needs some tweaks to be better aligned with the rest of the library (mostly scope handling & var naming). Needs a bit more investigation when I'll have the time. |
| transclude: true, | ||
| replace: true, | ||
| templateUrl: 'progressbar/progressbar.tpl.html', | ||
| scope:{ |
There was a problem hiding this comment.
AngularStrap uses scope: true accross the project, why use & for animate (and @ for type)?
There was a problem hiding this comment.
I was using & for animate because I didn't know optional 2 way bindings existed, I've updated it to =? :). What's the advantage of using scope:true?
There was a problem hiding this comment.
In the past, the isolated scopes were buggy, and leaked to other directives (see angular/angular.js/issues/1924). So AngularStrap avoided them since they were mostly decorator directives and this bug broke ngModel bindings.
It is now fixed but the project has not migrated yet, for the sake of consistency, I'd rather keep defaulting to scope:true (leaving the isolated scope definition in comments to ease a future update). At some point (maybe for the next minor), we should migrate all directives at once.
|
Is this available in the latest build? |
|
No, it hasn't been merged yet |
|
It would be nice if this change:
|
.progress-bar{
min-width: 2em;
}
<bs-progressbar value="value">My Content</bs-progressbar> |
|
Regarding the naming, I'm wondering if the naming shouldn't be |
|
Can't wait to see this PR merged. |
|
Hopefully this can be merged soon! |
|
+1. |
|
+1 |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
resolves #379 , adds a progress bar directive. Allows you to enable/disable animations and select the type of progress bar (success, warning, etc.).